Skip to content

WA-CI-005: Fix MountPoint.unwrap_app to stop at Class objects (next CI regression)#746

Merged
kitcommerce merged 1 commit intonextfrom
wa-ci-005-mount-point-fix
Mar 3, 2026
Merged

WA-CI-005: Fix MountPoint.unwrap_app to stop at Class objects (next CI regression)#746
kitcommerce merged 1 commit intonextfrom
wa-ci-005-mount-point-fix

Conversation

@kitcommerce
Copy link

Fixes the next branch CI regression introduced by PR #739.

Root Cause

MountPoint.unwrap_app called .app recursively without checking whether the current object is a Class. Rails Engine classes respond to .app (returning their routes), so unwrap_app traversed through the engine class and returned something deeper in the delegation chain — never the engine class itself.

Result: MountPoint.find(Workarea::Storefront::Engine) returned nil. Admin::StorefrontHelper#storefront then called send(nil)nil is not a symbol nor a string — crashing every admin test that renders the application layout.

Fix

Added return app if app.is_a?(Class) at the top of unwrap_app. Engine classes are Class objects; the ActionDispatch::Routing::Mapper::Constraints wrappers we want to peel are instances. This guard is backward-compatible with Rails 6.1 and Rails 7.

Verification

  • Existing MountPoint tests pass
  • Added test_unwrap_app_stops_at_class regression test
  • Admin integration tests (reports) pass locally with Docker services running
  • CI expected to go green on next after merge

Client impact

None — internal routing utility. Behavior of mount_point helper is restored to correct semantics.

The unwrap_app helper introduced in PR #739 was recursively calling .app
on Rails engine classes. Engine classes respond to .app (returning their
routes), causing unwrap_app to traverse past the engine class and never
return it. Result: MountPoint.find returned nil for all engines.

Fix: add `return app if app.is_a?(Class)` guard before the .app delegation
check. Engine classes are Class objects; the Constraints wrappers we want
to peel are instances. This is compatible with Rails 6.1 and Rails 7.

Fixes next-branch CI regression (broken since 2026-03-02 15:54 ET).
@kitcommerce kitcommerce added gate:build-pending Build gate running gate:build-passed Build gate passed review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress review:architecture-done Review complete review:simplicity-done Review complete review:security-done Review complete review:rails-conventions-done Rails conventions review complete and removed gate:build-pending Build gate running review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress labels Mar 3, 2026
@kitcommerce
Copy link
Author

Wave 1 Review Summary

All 4 Wave 1 reviewers have returned verdicts. Wave 1 gate: PASS

Reviewer Verdict Severity
architecture PASS
security PASS
simplicity PASS
rails-conventions PASS_WITH_NOTES LOW

architecture

Clean fix. Correct layer and scope — MountPoint internal utility. Type-based is_a?(Class) guard is sound and stable. Guard ordering appropriate. New test uses real engine class. No new dependencies or coupling. No changes needed.

security

No security implications. Pure control-flow guard on internal Rack middleware unwrapping utility. No new input handling, deserialization, or network calls. Existing depth guard retained.

simplicity

Single guard clause addition — minimal surface area. Inline comment explains the non-obvious Class check clearly. Test is direct and minimal. Follows existing guard-then-recurse pattern exactly.

rails-conventions (PASS_WITH_NOTES)

Two LOW-severity documentation style nits — neither blocks merge:

  1. New YARD doc drops +…+ inline-code delimiters used elsewhere in the file.
  2. Inline comment is slightly verbose for a one-liner guard — could be consolidated into the YARD doc.

Proceeding to Wave 2 (rails-security, database, test-quality).

@kitcommerce kitcommerce added review:rails-security-pending Rails security review in progress review:database-pending Database review in progress review:test-quality-pending Review in progress review:test-quality-done Review complete review:rails-security-done Rails security review complete review:database-done Database review complete and removed review:test-quality-pending Review in progress review:rails-security-pending Rails security review in progress review:database-pending Database review in progress labels Mar 3, 2026
@kitcommerce
Copy link
Author

Wave 2 Review Results — All Passed ✅

rails-security: PASS — Zero security surface; internal routing utility guard clause only. No user input, no auth, no views.

database: PASS — No database-related changes in this PR. Pure Ruby routing utility only.

test-quality: PASS_WITH_NOTES (LOW) — New regression test test_unwrap_app_stops_at_class directly covers the fix, well-named and deterministic. Two optional coverage notes (nested-depth test, explicit middleware chain test) — non-blocking.

All Wave 2 reviewers passed. Proceeding to Wave 3 (performance, frontend, accessibility).

@kitcommerce kitcommerce added review:performance-pending Review in progress review:frontend-pending Frontend review in progress review:accessibility-pending Review in progress labels Mar 3, 2026
@kitcommerce kitcommerce added review:performance-done Review complete review:frontend-done Frontend review complete review:accessibility-done Review complete and removed review:performance-pending Review in progress review:accessibility-pending Review in progress review:frontend-pending Frontend review in progress labels Mar 3, 2026
@kitcommerce
Copy link
Author

Wave 3 Review Results — All PASS ✅

performance (PASS): Trivially cheap is_a?(Class) guard terminates recursion earlier in Engine cases. Net positive — no allocation, no IO, no hot-path impact.

accessibility (PASS): No accessibility surface area — pure backend Ruby routing utility change. No UI, views, HTML, CSS, or JS touched.

frontend (PASS): No frontend code in this diff — pure Ruby files only. Rack app-traversal utility with no client-side surface area.

Wave 3 complete. Proceeding to Wave 4 (documentation).

@kitcommerce kitcommerce added review:documentation-pending review:documentation-done merge:ready All conditions met, eligible for merge merge:hold In hold window before auto-merge and removed review:documentation-pending labels Mar 3, 2026
@kitcommerce
Copy link
Author

✅ All Review Waves Passed

All reviewers returned PASS or PASS_WITH_NOTES. This PR is merge-ready.

  • Wave 1 (Foundation — architecture, simplicity, security, rails-conventions): ✅ All PASS
  • Wave 2 (Correctness — rails-security, database, test-quality): ✅ All PASS
  • Wave 3 (Quality — performance, accessibility, frontend): ✅ All PASS
  • Wave 4 (Documentation): ✅ PASS — minor docstring precision nit (non-blocking)

Documentation reviewer note: Docstring could be marginally more precise — unwrap_app may return a Class that responds to :app; "innermost non-wrapper app" is slightly ambiguous but acceptable.

Labeled merge:ready + merge:hold. Hold window: 60 minutes from ~15:39 ET. Auto-merge eligible after ~16:39 ET.

@kitcommerce kitcommerce merged commit 598997f into next Mar 3, 2026
7 of 15 checks passed
@kitcommerce kitcommerce deleted the wa-ci-005-mount-point-fix branch March 3, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate:build-passed Build gate passed merge:hold In hold window before auto-merge merge:ready All conditions met, eligible for merge review:accessibility-done Review complete review:architecture-done Review complete review:database-done Database review complete review:documentation-done review:frontend-done Frontend review complete review:performance-done Review complete review:rails-conventions-done Rails conventions review complete review:rails-security-done Rails security review complete review:security-done Review complete review:simplicity-done Review complete review:test-quality-done Review complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant